Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented VS API for toggling of C++20 modules and STL module build #2130

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

nickclark2016
Copy link
Member

What does this PR do?

Closes #2105 by introducing 2 new APIs. Introduces buildstlmodules and enablemodules to the vsXXXX exporters. buildstlmodules controls if the STL modules should be built for C++23 and enablemodules controls if modules are to be built. This API does not affect if module files are added to the project or not.

Example Usage:

buildstlmodules 'Off' -- affects VS2022 only
enablemodules 'On' -- affects VS2019 and VS2022

How does this PR change Premake's behavior?

Does not affect existing scripts.

Anything else we should know?

N/A

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

website/docs/buildstlmodules.md Outdated Show resolved Hide resolved
website/docs/enablemodules.md Show resolved Hide resolved

function m.buildStlModules(cfg)
if _ACTION >= "vs2022" then
if cfg.buildstlmodules then
Copy link
Contributor

@Jarod42 Jarod42 Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have this one activated, and not the other? (to emit warning for strange config).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, it makes no sense to have this activated wtihout the other, but I don't believe we do this kind of sanity checking elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@vulcan-dev
Copy link

I appreciate you adding this, makes it easier for a lot of us to experiment and or use it.
However, I do have a question regarding ScanSourceForModuleDependencies, I've had to enable that for anything to really work, otherwise I can't import things from the standard library or Windows.h. Do you think you could possibly add that if you have free time? If not, then I think I could add it soon because I see how you've added the others.

@nickclark2016
Copy link
Member Author

I appreciate you adding this, makes it easier for a lot of us to experiment and or use it. However, I do have a question regarding ScanSourceForModuleDependencies, I've had to enable that for anything to really work, otherwise I can't import things from the standard library or Windows.h. Do you think you could possibly add that if you have free time? If not, then I think I could add it soon because I see how you've added the others.

Does scanformoduledependencies fit your needs? It is an existing API.

@nickclark2016
Copy link
Member Author

I appreciate you adding this, makes it easier for a lot of us to experiment and or use it. However, I do have a question regarding ScanSourceForModuleDependencies, I've had to enable that for anything to really work, otherwise I can't import things from the standard library or Windows.h. Do you think you could possibly add that if you have free time? If not, then I think I could add it soon because I see how you've added the others.

Does scanformoduledependencies fit your needs? It is an existing API.

Bumping this @vulcan-dev

@vulcan-dev
Copy link

I appreciate you adding this, makes it easier for a lot of us to experiment and or use it. However, I do have a question regarding ScanSourceForModuleDependencies, I've had to enable that for anything to really work, otherwise I can't import things from the standard library or Windows.h. Do you think you could possibly add that if you have free time? If not, then I think I could add it soon because I see how you've added the others.

Does scanformoduledependencies fit your needs? It is an existing API.

Bumping this @vulcan-dev

I apologise, I must have had to go when I was responding last time.

It does not, I remember having issues regarding modules, and I had to enable the other option. In my temporary fix, I have it enabled as well and that's what worked for me.

@nickclark2016
Copy link
Member Author

I appreciate you adding this, makes it easier for a lot of us to experiment and or use it. However, I do have a question regarding ScanSourceForModuleDependencies, I've had to enable that for anything to really work, otherwise I can't import things from the standard library or Windows.h. Do you think you could possibly add that if you have free time? If not, then I think I could add it soon because I see how you've added the others.

Does scanformoduledependencies fit your needs? It is an existing API.

Bumping this @vulcan-dev

I apologise, I must have had to go when I was responding last time.

It does not, I remember having issues regarding modules, and I had to enable the other option. In my temporary fix, I have it enabled as well and that's what worked for me.

So in the fix from this code, the scanformoduledependencies does not work? That's weird, since that targets the ScanSourceForModuleDependencies property.

@vulcan-dev
Copy link

I appreciate you adding this, makes it easier for a lot of us to experiment and or use it. However, I do have a question regarding ScanSourceForModuleDependencies, I've had to enable that for anything to really work, otherwise I can't import things from the standard library or Windows.h. Do you think you could possibly add that if you have free time? If not, then I think I could add it soon because I see how you've added the others.

Does scanformoduledependencies fit your needs? It is an existing API.

Bumping this @vulcan-dev

I apologise, I must have had to go when I was responding last time.

It does not, I remember having issues regarding modules, and I had to enable the other option. In my temporary fix, I have it enabled as well and that's what worked for me.

So in the fix from this code, the scanformoduledependencies does not work? That's weird, since that targets the ScanSourceForModuleDependencies property.

If that's the case, then it would work. I thought it was a different option, but I thought wrong. My apologies

@nickclark2016
Copy link
Member Author

If that doesn't work when we merge this in, open up a bug and I'll look.

Comment on lines +201 to +205
kind = "string",
allowed = {
"On",
"Off"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the boolean type would probably be better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about using those, but there are also plenty of spots in the code where "Off" and "On" are used in place of a boolean value. Should we start just using boolean instead? If so, that's an easy fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inputs for boolean are more flexible:

local mapping = {
["false"] = false,
["no"] = false,
["off"] = false,
["on"] = true,
["true"] = true,
["yes"] = true,
}
if type(value) == "string" then
value = mapping[value:lower()]
if value == nil then
error { msg="expected boolean; got " .. value }
end
return value
end
if type(value) == "boolean" then
return value
end
if type(value) == "number" then
return (value ~= 0)
end
return (value ~= nil)

From memory there are issues when using this with look up tables, which is how the toolsets work. So, it's up to you whether you think it's a good idea or not.

Comment on lines +211 to +215
kind = "string",
allowed = {
"On",
"Off"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@nickclark2016 nickclark2016 merged commit b94b9d7 into premake:master Oct 30, 2023
14 checks passed
@HerrDingenz
Copy link

did this got implemented in a new release already?

@nickclark2016
Copy link
Member Author

did this got implemented in a new release already?

No new release has been cut since this. You can either use binaries from a recent GitHub Action, or you could build premake yourself

@HerrDingenz
Copy link

nevermind - i just noticed it will be in Premake 5.0.0 beta 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggling Visual Studio's new C++ Modules on/off in premake lua script
5 participants